Skip to content

Conversation

paulmedynski
Copy link
Contributor

@paulmedynski paulmedynski commented Aug 14, 2025

Abandoned in favour of #3626 and #3628.

Overview

As part of the Azure split work, a new Abstractions package is being created that contains types shared between MDS and its future extensions (Azure will be the first). This PR creates that package (with no meaningful content) to setup the MDS dependency, and get testing and CI setup accordingly.

I'm also experimenting with simplified .slnx files and some project directory structure changes.

This supercedes the original draft PR #3471.

Issues

The first step towards #1108.

Testing

  • Unit tests for the sample class to prove that CI can run them successfully.
  • There are no MDS tests to prove out the new dependency yet. Those will naturally happen when the first real bit of Azure functionality is moved out into the new Azure extension package.

@paulmedynski paulmedynski added this to the 7.0-preview1 milestone Aug 14, 2025
@paulmedynski paulmedynski force-pushed the dev/paul/azure-split/abstractions branch from e912975 to 261f4af Compare August 14, 2025 17:25
Copy link
Contributor Author

@paulmedynski paulmedynski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding comentary for reviewers.

I will be renaming the Extensions package to Abstractions. That round of changes is next.

Copy link
Contributor Author

@paulmedynski paulmedynski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commentary for reviewers, and issues for myself to fix.

Copy link

codecov bot commented Aug 15, 2025

Codecov Report

❌ Patch coverage is 53.24675% with 180 lines in your changes missing coverage. Please review.
✅ Project coverage is 69.96%. Comparing base (fd16c6d) to head (50c52c1).
⚠️ Report is 6 commits behind head on feat/azure-split.

Files with missing lines Patch % Lines
...nt/netfx/src/Microsoft/Data/SqlClient/TdsParser.cs 23.93% 89 Missing ⚠️
.../netcore/src/Microsoft/Data/SqlClient/TdsParser.cs 29.52% 74 Missing ⚠️
...lClient/src/Microsoft/Data/SqlClient/SqlCommand.cs 90.06% 15 Missing ⚠️
...ient/src/Microsoft/Data/SqlClient/SqlStatistics.cs 77.77% 2 Missing ⚠️
Additional details and impacted files
@@                 Coverage Diff                  @@
##           feat/azure-split    #3567      +/-   ##
====================================================
- Coverage             70.67%   69.96%   -0.71%     
====================================================
  Files                   277      276       -1     
  Lines                 61686    61641      -45     
====================================================
- Hits                  43598    43130     -468     
- Misses                18088    18511     +423     
Flag Coverage Δ
addons 90.82% <ø> (ø)
netcore 73.75% <65.13%> (+0.39%) ⬆️
netfx 68.63% <61.31%> (-0.95%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@paulmedynski paulmedynski marked this pull request as ready for review August 15, 2025 16:56
@paulmedynski paulmedynski requested a review from a team as a code owner August 15, 2025 16:56
@paulmedynski paulmedynski changed the title [WIP] Create Abstractions package Create Abstractions package Aug 15, 2025
@apoorvdeshmukh apoorvdeshmukh self-assigned this Aug 27, 2025
@paulmedynski paulmedynski force-pushed the dev/paul/azure-split/abstractions branch from c2a8e97 to d4464ec Compare August 29, 2025 12:06
- Added empty Extensions package with some sample class and docs to demonstrate packaging.
- Created CI stage to build, test, pack, and publish the Extensions NuGet package.
- Updated downstream CI stages/jobs to use the Extensions package.
- Updated build.proj Clean target to not delete packages/ dir.
- Updated BUILDGUIDE with instructions for the Extensions package.
- Cleaned up stale BUIDGUIDE sections.
- Added temporary GitHub Discussion content so the team can review before posting it as a real Discussion.
- Disable .pdb file inclusion in the application package.
- Renamed Extensions package to Abstractions.
- Updated README related to extensions design.
@paulmedynski paulmedynski force-pushed the dev/paul/azure-split/abstractions branch from d4464ec to f6bbbaf Compare September 5, 2025 14:27
Copy link
Contributor

@benrr101 benrr101 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm omitting all my comments about directory structure as per offline discussion. But I think we want to be really careful and thoughtful with how we set up pipelines, build targets, and project structure.

paulmedynski and others added 5 commits September 15, 2025 09:44
- Addressed some of the review comments.
- Fixed missing Abstractions package version.
* Task 38530: Prepare release notes

- Added 7.0.0 Preview 1 release notes.

* Task 38530: Prepare release notes

- Addressed review comments.

* Task 38530: Prepare release notes

- Filled in actual build number.
Copy link
Contributor

@benrr101 benrr101 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awaiting the rest of the changes, but we're definitely making progress

edwardneal and others added 6 commits September 16, 2025 17:39
* netcore, netfx: sync declarations of TdsOperationStatus variables

* netcore: reorder TryProcessUDTMetadata

* netfx: reorder WriteTceFeatureRequest

* netfx: reorder WriteAzureSQLSupportFeatureRequest

* netcore, netfx: merge WriteInt and SerializeInt

netcore: removed a simple WriteInt which did nothing besides call BinaryPrimitives.
netcore: when there's space in the packet buffer, WriteInt will now write to it directly (rather than write to a stack-allocated span and copy.)

* netcore: remove ConstructGuid method

* netcore, netfx: refactor and sync serialization of guids

* netcore, netfx: refactor and sync serialization of floats and doubles

This removes the need for TdsParser.netcore.cs.

* netfx: adjust TraceString - parameter was being passed to it which was never referenced in the format string

* netcore: sync exception messages with netfx

Messages of the netfx exceptions are more detailed

* netfx, netcore: centralise masking of received server options

Added mask to netcore logic.
Also removed mask on _encryptionOption from netfx - this will never be outside the range of EncryptionOptions.OPTIONS_MASK.

* netcore: enforce server certificate validation if AccessTokenCallback is set and certificate is not automatically trusted

* netfx: add static lambda to TdsExecuteSQLBatch

* netfx: remove failed attempt at static lambda

This passed state around, but never passed enough state to remove the state machine. Sync with netcore.

* netfx: sync reference to length of JSON metadata substitution sequence

* netfx: pre-PR correction: match previous behaviour when writing Guid instances from WriteUnterminatedSqlValue

* Flip order of debug assertion

* Next round of code review

Remove unnecessary conditional compilation.
Add XML documentation to clarify SerializeShort, WriteShort et al.

* netcore, netfx: sync TraceString

Both platforms will now show the TransparentNetworkIPResolution status. This is always disabled on netcore.

* Expand ParametersTest to include SqlGuid (including null value)

* netfx: port RequestContinue call from netcore
- Changed MDS to reference Abstractions via project or package depending on build parameters.
- Removed obsolete package reference files.
- Updated CI pipelines to build Abstractions in parallel for Project-based builds.
- Modularized Abstractions CI templates.
- Cleaned up the project vs package pathways.
* Merge:
* _parameters
* _pendingCancel
* _preparedConnectionCloseCount
* _preparedConnectionReconnectCount
* _prepareHandle
* s_cachedInvalidPrepareHandle
* Statistics

* Merge Connection, DbConnection

* Merging RetryLogicProvider and _retryLogicProvider

# Conflicts:
#	src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlCommand.cs

* Merge Transaction, DbTransaction

* Merge CommandText
* Replace "" with string.Empty

* Merge CommandTimeout, _commandTimeout, and ADP.InvalidCommandTimeout

* Merge CommandType
* Removed temp storage of _commandType

* Merge ColumnEncryptionSetting, _columnEncryptionSetting

* Merge Parameters and DbParameterCollection

* Merging DesignTimeVisible, _designTimeVisible

* Merging EnableOptimizedParameterBinding, UpdatedRowSource, _updatedRowSource

* Merge StatementCompleted and _statementCompletedEventHandler

* Merging Notification, NotificationAutoEnlist, _notification, _notificationAutoEnlist, _sqlDep

* Merge DefaultCommandTimeout, InternalTdsConnection, IsColumnEncryptionEnabled, InternalRecordsAffected, _rowsAffected, PropertyChanging()

* merge _transaction and _batchRPCMode

* Restore ResetCommandTimeout that got accidentally obliterated in a previous commit

* Fix logic issue in IsColumnEncryptionEnabled, reinstate TypeDescriptor.Refresh on netfx
paulmedynski and others added 7 commits September 18, 2025 08:35
- Replaced generic "NugetPackageVersion" with variables names specific to the package (i.e. MdsPackageVersion, AkvPackageVersion, etc).
- Removed top-level Abstractions package version parameters in favour of a variable.
- Fixes to Abstractions version number construction.
- Fixes for some build.proj targets.
- Fixes for Abstractions and AKV default versioning.
- Added missing CI variable $(abstractionsPackageVersion)
- Fixed missing parameter when packing Abstractions.
* Expand code coverage for UDT serialization

* Address test failures on 32-bit OSes

* Make Microsoft.SqlServer.Server a PackageReference

* Reformat SerializedTypes.cs

* Switch to implicit object creation

Also enforce this via editorconfig

* Add and correct comments

Also mandating this by generating a documentation XML file

* Enable nullable for project

* Declare and assign variables together

* Moved MemoryStream to an instance variable

* Post-merge build corrections

* Reapply IDE0090 recommendations, disable CS1591 on newly-added tests

* Next code review (first response)

* Switch to using TheoryData
* Nit: => movement
* Rename tests
* Split delegate for Assert.Throws into separate arrange/act/assert segment of each method

* Disable CS1591 on new tests

* Disable mandatory XML documentation

* Change formatting of test data

* Remove trailing XML documentation override
- Removed obsolete ADO Library inclusion in CI variables.
- Moved MDS CI versions numbers into CI variables.
- Fixed Abstractions versioning to handle CI version format.
Copy link
Contributor Author

@paulmedynski paulmedynski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding commentary for reviewers, and some items for me to address.

variables:
- template: ../../../libraries/variables.yml@self
- ${{ if parameters.isPreview }}:
- name: NugetPackageVersion
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found NugetPackageVersion confusing - which nuget package? So I gave them names specific to the package. I didn't rename any of these yml files to match such changes though. I figure that's more trouble than it's worth, and we intend to re-factor the whole works here soon anyway.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to me. I appreciate that kind of verbosity, and it was something I aimed to resolve in the AKV provider pipeline. Needs to be spread to other pipelines :D

Get-Content -Path "NuGet.config"
displayName: 'Read NuGet.config [debug]'
- task: DotNetCoreCLI@2
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure why this was here, so I removed it.

arguments: 'build.proj -t:restore'
feedsToUse: 'select'

- powershell: |
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AKV .csproj already has conditional references depending on Project vs Package, so this wasn't necessary.

parameters:
SNIVersion: ${{parameters.SNIVersion}}
SNIValidationFeed: ${{parameters.SNIValidationFeed}}
- template: common/templates/steps/ci-prebuild-step.yml@self
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This and the else block are identical, so I pulled them out.

- Addressed my own review comments.
- Fixed parameter typo in YAML.
benrr101
benrr101 previously approved these changes Sep 19, 2025
Copy link
Contributor

@benrr101 benrr101 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think this is a good improvement over the OG PR. We'll mess with it some more as we get time for it 👍

- Fixed Abstractions package versioning in official builds.
- Fixed broken assembly version checks in official builds.
- Removed Abstractions solution file.
- Fixing invalid package/assembly versions due to build numbers containing dots.
- Fixed official build assembly versioning.
- Fixed incorrect variable dereference syntax in expression.
- Fixed assembly versions > 65534.
- Improving diagnostics for assembly version checks.
- Trying to fix PowerShell variable expansion and assembly version checks.
- Diagnostics to figure out why MDS assembly file versions aren't being set.
- Added empty Extensions package with some sample class and docs to demonstrate packaging.
- Created CI stage to build, test, pack, and publish the Extensions NuGet package.
- Updated downstream CI stages/jobs to use the Extensions package.
- Updated build.proj Clean target to not delete packages/ dir.
- Updated BUILDGUIDE with instructions for the Extensions package.
- Cleaned up stale BUIDGUIDE sections.
- Added temporary GitHub Discussion content so the team can review before posting it as a real Discussion.
- Disable .pdb file inclusion in the application package.
- Renamed Extensions package to Abstractions.
- Updated README related to extensions design.
- Changed MDS to reference Abstractions via project or package depending on build parameters.
- Removed obsolete package reference files.
- Updated CI pipelines to build Abstractions in parallel for Project-based builds.
- Modularized Abstractions CI templates.
- Cleaned up the project vs package pathways.
- Replaced generic "NugetPackageVersion" with variables names specific to the package (i.e. MdsPackageVersion, AkvPackageVersion, etc).
- Removed top-level Abstractions package version parameters in favour of a variable.
- Fixes to Abstractions version number construction.
- Fixes for some build.proj targets.
…/SqlClient into dev/paul/azure-split/abstractions
- Fixed duplicate parameters from git merge.
@paulmedynski
Copy link
Contributor Author

Abandoning in favour of #3626 and #3628.

@paulmedynski paulmedynski deleted the dev/paul/azure-split/abstractions branch September 25, 2025 16:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants